fix: Synchronize charge operations to prevent race conditions #684
Conversation
github-actions
bot
added
the
t-tooling
src/apify/_charging.py
Outdated
| # START OF CRITICAL SECTION - no awaits here | ||
| # Acquire lock to prevent race conditions between concurrent charge calls | ||
| # (e.g., when Actor.push_data with charging is called concurrently with Actor.charge). | ||
| async with self._charge_lock: |
There was a problem hiding this comment.
This, on its own, doesn't fix the issue of multiple interleaved, PPE-aware Actor.push_data calls. The lock would need to be held for the whole duration of the push_data+charge sequence.
There was a problem hiding this comment.
Thanks, so I moved the lock to the Actor class, take a look.
vdusek
force-pushed
the
sync-actor-charge-calls
branch
from
976e481 to
e34aeed
Compare
| pushed_items_count = min(max_charged_count, len(data)) if max_charged_count is not None else len(data) | ||
| # If charging is requested, acquire the charge lock to prevent race conditions between concurrent | ||
| # push_data calls. We need to hold the lock for the entire push_data + charge sequence. | ||
| async with self._charge_lock: |
There was a problem hiding this comment.
In theory, this could still get interleaved with an Actor.charge call with an unrelated event, which would result in pushing more items than the user can afford.
A watertight solution would be to use a read-write lock where plain Actor.charge would acquire the read-lock and Actor.push_data would acquire the write-lock.
But since there is no built-in read-write lock for asyncio, we should probably just leave this the way it is 🙂
Closes: #666